Management command for inactive user purge#6803
Management command for inactive user purge#6803smithellis wants to merge 3 commits intomozilla:mainfrom
Conversation
akatsoulas
left a comment
akatsoulas
left a comment
There was a problem hiding this comment.
How is this management command different from cleanup_expired_users? Is this going to be a one off deletion or are we going to reuse it?
| try: | ||
| user_count = deleted_objects.get("auth.User", 0) | ||
| return user_count, 0 | ||
| except (DatabaseError, IntegrityError) as e: |
There was a problem hiding this comment.
Why are we using these 2 exceptions here?
There was a problem hiding this comment.
These should be covering the actual .delete() vs. the retrieval of the user count. I'll move the deletion activity into the try. Good catch - thanks!
There was a problem hiding this comment.
My point was mostly about why these two specific exceptions. Why not for example OperationalError (which we have in all environments). Shall we just use Exception here?
There was a problem hiding this comment.
They seemed more specific to what could go wrong, but maybe being more general makes sense. I'll alter to use Exception.
Thanks!
|
This was initially dealing with Migration to bulk delete inactive users regardless of content which we decided to then approach as a management command which then got reverted due to a polluted commit.
|
akatsoulas
left a comment
akatsoulas
left a comment
There was a problem hiding this comment.
r+wc please hold on merging until the PR to adjust contributor metrics calculation is live
| def _bulk_delete_batch(self, User, user_ids): | ||
| try: | ||
| _, deleted_objects = User.objects.filter(id__in=user_ids).delete() | ||
| user_count = deleted_objects.get("auth.User", 0) |
There was a problem hiding this comment.
this line and the next should be better in the else clause of a try/except/else
There was a problem hiding this comment.
You are correct! Thanks!
| try: | ||
| user_count = deleted_objects.get("auth.User", 0) | ||
| return user_count, 0 | ||
| except (DatabaseError, IntegrityError) as e: |
There was a problem hiding this comment.
My point was mostly about why these two specific exceptions. Why not for example OperationalError (which we have in all environments). Shall we just use Exception here?
2 participants
No description provided.